-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[INFRA] Drop gcc9 #2952
[INFRA] Drop gcc9 #2952
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/seqan/seqan3/3qZfjUW2qxe5pYjdwmcygai3sxTV |
Codecov Report
@@ Coverage Diff @@
## master #2952 +/- ##
=======================================
Coverage 98.22% 98.22%
=======================================
Files 267 267
Lines 11494 11494
=======================================
Hits 11290 11290
Misses 204 204
Continue to review full report at Codecov.
|
4567eca
to
406fe39
Compare
e2c803f
to
22cf747
Compare
22cf747
to
3a75e0b
Compare
3a75e0b
to
cd3e854
Compare
1058327
to
e0b6ef2
Compare
e0b6ef2
to
ea6c513
Compare
ea6c513
to
32a56b7
Compare
32a56b7
to
6085d78
Compare
6085d78
to
ef82f7b
Compare
ef82f7b
to
3049495
Compare
3049495
to
d081e66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of changes. My only real remark is my last annotation. Something about a shim-implementation for floating types. I didn't get that part :/
The rest looks good.
I also added a few remarks here and there, that arn't really part of the PR, but I would also never look at it again and wasn't sure if it is correct.
f = range; | ||
#endif | ||
return std::move(f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is return f
not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷
It is definitely needed, because I tried to remove it :D
Copy elision doesn't take effect here.
I think this is some code where we have overloads for each const
&
&&
combo?
{insert_gap(v, std::ranges::begin(v), 2)} -> std::same_as<std::ranges::iterator_t<t>>; | ||
{erase_gap(v, std::ranges::begin(v))} -> std::same_as<std::ranges::iterator_t<t>>; | ||
{erase_gap(v, std::ranges::begin(v), std::ranges::end(v))} -> std::same_as<std::ranges::iterator_t<t>>; | ||
{assign_unaligned(v, unaligned)} -> std::same_as<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks so much nicer now!
@@ -230,7 +230,7 @@ class inherited_iterator_base : public std::conditional_t<std::is_pointer_v<base | |||
constexpr derived_t operator++(int) noexcept(noexcept(std::declval<base_t &>()++) && | |||
noexcept(derived_t(std::declval<base_t &>()))) | |||
//!\cond | |||
requires requires (base_t_ i) { i++; SEQAN3_RETURN_TYPE_CONSTRAINT(i++, std::same_as, base_t_); } && | |||
requires requires (base_t_ i) { i++; {i++} -> std::same_as<base_t_>; } && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we leave the first i++ away?
requires requires (base_t_ i) { {i++} -> std::same_as<base_t_>; } &&
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but I wouldn't change the concept in this PR.
Theoretically, it first checks whether i++
is valid, and then whether it returns the correct type. The first i++
is probably not needed.
SEQAN3_RETURN_TYPE_CONSTRAINT(std::swap(val, val2), std::same_as, void); | ||
{val.swap(val2)} -> std::same_as<void>; | ||
{swap(val, val2)} -> std::same_as<void>; | ||
{std::swap(val, val2)} -> std::same_as<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find where it says that std::swap has to be possible by the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really an STL concept anyway.
https://en.cppreference.com/w/cpp/algorithm/swap#Specializations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope these concepts are in detail? They are not really usable as one usually only needs a container to be e.g. "push-backable" but not everything else. I think this was already decided but never done
@@ -66,7 +66,7 @@ Please see the [online documentation](https://docs.seqan.de/seqan/3-master-user/ | |||
|
|||
| | requirement | version | comment | | |||
|-------------------|------------------------------------------------------|----------|---------------------------------------------| | |||
|**compiler** | [GCC](https://gcc.gnu.org) | ≥ 7 | no other compiler is currently supported! | | |||
|**compiler** | [GCC](https://gcc.gnu.org) | ≥ 10 | no other compiler is currently supported! | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
#include <utility> // __cpp_lib_to_chars may be defined here as currently documented | ||
#include <charconv> | ||
#include <utility> // __cpp_lib_to_chars may be defined here as currently documented. | ||
#include <version> // From C++20 onwards, all feature macros should be defined here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the comment after the include is not required anymore.
|
||
// ========================================================================= | ||
// If any std implementation is present use that as basis (>= gcc8) | ||
// If float implementation is missing, add our own shim-implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should still be any std
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int
is provided for gcc >= 10, float
for >= 11, I think.
I removed the int shim, so we only ever add float
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go and drop gcc9!!!!
No description provided.